Pass locations of external dependencies through arguments#121
Pass locations of external dependencies through arguments#121jstma wants to merge 1 commit intoaspiers:masterfrom
Conversation
|
Addresses issue #120 |
ly2video/cli.py
Outdated
| group_os = parser.add_argument_group(title='External programs') | ||
|
|
||
| group_os.add_argument( | ||
| "--windows-lilypond", dest="winLilypond", |
There was a problem hiding this comment.
Does this have to be Windows-specific? Why not just allow optionally specifying an arbitrary path for where to find lilypond, e.g. --lilypond="C:\\lilypond\\bin\\lilypond"? This would allow greater flexibility on other operating systems too.
| ffmpeg = options.winFfmpeg + "ffmpeg" | ||
| if os.system(ffmpeg + " -version" + redirectToNull) != 0: | ||
| fatal("FFmpeg was not found (maybe use --windows-ffmpeg?).", 2) | ||
| stdout = safeRun(["ffmpeg", "-version", redirectToNull], |
There was a problem hiding this comment.
Similarly here, why not just have --ffmpeg and --timidity options which allow pointing to those executables in any location on any OS?
ly2video/cli.py
Outdated
| if not options.winLilypond: | ||
| if 'lilypond-windows' in section: | ||
| options.winLilypond = section['lilypond-windows'] | ||
| if not options.winFfmpeg: | ||
| if 'ffmpeg-windows' in section: | ||
| options.winFfmpeg = section['ffmpeg-windows'] | ||
| if not options.winTimidity: | ||
| if 'timidity-windows' in section: | ||
| options.winTimidity = section['timidity-windows'] |
There was a problem hiding this comment.
Similarly here, I think there's probably no reason to limit this to Windows. Also if an option is already set on the CLI, it shouldn't be overridden by ly2video.cfg; the CLI option should take precedence as this is conventional behaviour.
There was a problem hiding this comment.
Similarly here, I think there's probably no reason to limit this to Windows.
Agreed, I was just following what was originally there. I'll get rid of all the win references.
Also if an option is already set on the CLI, it shouldn't be overridden by
ly2video.cfg; the CLI option should take precedence as this is conventional behaviour.
I agree, and said in those comments, it was for testing and to start the conversation about ConfigArgParse. It's an external module that does file and argument parsing.
Otherwise I need to think of a clean way to parse the file first and conditionally apply to options. That's going to be some ugly code.
If this were c I would do it like this:
struct { } options;
parse_file(&options);
parse_args(&options);
This way the CLI arguments will overwrite the file arguments. I don't know the 'python' way to do this using configparser and argparse. Suggestions welcome.
There was a problem hiding this comment.
Oh, I only just noticed that you already do the check whether the option is set, so that already prevents it being overridden 👍
That said, you could shorten it by skipping both if conditionals, e.g.
options.ffmpeg = options.ffmpeg or section.get('ffmpeg')Unfortunately Python doesn't have an equivalent of the ||= operator, otherwise it could have been shortened to
options.ffmpeg ||= section.get('ffmpeg')
* pass locations of external dependencies through arguments * use ly2video.cfg to bring in windows paths for lilypond, ffmpeg, and timidity. Could probably be used on other OSs too. * ly2video.cfg works on Windows in Wine, not tested on other OSs. * works properly on linux if ly2video.cfg is not defined * don't run convert.ly on windows. The process fails for an unknown reason. * tmpPath does not store the path from the last time ly2video was run. Since the temporary directory name is random, there's no point in deleting 'old temporary files'. * tmpPath was using RUNDIR for some strange reason. Though at execution time, it's "" which leaves just the temporary directory and the desired subdirectory name to join. * detect exception and print a message to the user. Not sure if this is only a wine problem. Will need someone to test on Windows. * Added some details regarding convert-ly.py on Windows
jstma
force-pushed
the
better-win-support
branch
from
c3f9fea to
76d78d4
Compare
2 participants
use ly2video.cfg to bring in windows paths for lilypond, ffmpeg, and timidity. Could probably be used on other OSs too.
ly2video.cfg works on Windows in Wine, not tested on other OSs.
works properly on linux if ly2video.cfg is not defined